Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove recursion from VMInputStream #2816

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hduelme
Copy link
Contributor

@hduelme hduelme commented Aug 12, 2024

What it does

Currently VMInputStream uses recursion to retry reads in the int read() method. I replaced the recursive call with a while loop. This removes unnecessary overhead while running tests.

How to test

This doesn't change any behavior.

Author checklist

@stephan-herrmann
Copy link
Contributor

This removes unnecessary overhead while running tests.

Can you give numbers for this overhead?

@hduelme
Copy link
Contributor Author

hduelme commented Aug 14, 2024

I did some testing. If the build is successful no recursive calls happen. Only if something is written to the errorStream the IOException is thrown. In this case a found between 10 to 500 recursive calls.

@stephan-herrmann
Copy link
Contributor

In this case a found between 10 to 500 recursive calls.

What's wrong with that?

When you said "overhead" I was expecting some time measurements... a recursive call is not overhead per se.

@hduelme
Copy link
Contributor Author

hduelme commented Aug 16, 2024

I agree. I was also expecting different results. I got here in the first place due to a exception with a large stacktrace

return read();
}
throw e;
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this would be an endless loop instead of Stackoverflow? Doesn't sound better, but yes the recursion seems wrong too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants